646 use graph model with ABMs v3 #1085

jubicker · 2024-08-01T13:00:24Z

Changes and Information

Please briefly list the changes (main added features, changed items, or corrected bugs) made:

add node property and edge property class for ABM graph simulation
add model wrapper for abm that is used for ABM graph simulation and can handle persons that commute to other nodes
add tests for new functionality
add graph abm example

Merge Request - Guideline Checklist

Please check our git workflow. Use the draft feature if the Pull Request is not yet ready to review.

Checks by code author

Every addressed issue is linked (use the "Closes #ISSUE" keyword below)
New code adheres to coding guidelines
No large data files have been added (files should in sum not exceed 100 KB, avoid PDFs, Word docs, etc.)
Tests are added for new functionality and a local test run was successful (with and without OpenMP)
Appropriate documentation for new functionality has been added (Doxygen in the code and Markdown files if necessary)
Proper attention to licenses, especially no new third-party software with conflicting license has been added
(For ABM development) Checked benchmark results and ran and posted a local test above from before and after development to ensure performance is monitored.

Checks by code reviewer(s)

Corresponding issue(s) is/are linked and addressed
Code is clean of development artifacts (no deactivated or commented code lines, no debugging printouts, etc.)
Appropriate unit tests have been added, CI passes, code coverage and performance is acceptable (did not decrease)
No large data files added in the whole history of commits(files should in sum not exceed 100 KB, avoid PDFs, Word docs, etc.)
On merge, add 2-5 lines with the changes (main added features, changed items, or corrected bugs) to the merge-commit-message. This can be taken from the briefly-list-the-changes above (best case) or the separate commit messages (worst case).

…/SciCompMod/memilio into 646-use-graph-model-with-abms-v2

…/SciCompMod/memilio into 646-use-graph-model-with-abms-v3

DavidKerkmann

Thanks for the sophisticated implementation!
In addition to a couple of small things, we should have a more detailed look at two things:

History embedding in ABM graph simulation
RNG embedding in ABM graph simulation
Details can be found in the comments.

cpp/models/graph_abm/graph_abm_mobility.h

DavidKerkmann · 2025-01-06T12:33:25Z

cpp/examples/graph_abm.cpp

+    model1.parameters.get<mio::abm::IncubationPeriod>()              = 4.;
+    model1.parameters.get<mio::abm::InfectedNoSymptomsToSymptoms>()  = 2.;
+    model1.parameters.get<mio::abm::InfectedNoSymptomsToRecovered>() = 4.;
+    model1.parameters.get<mio::abm::InfectedSymptomsToRecovered>()   = 5.;
+    model1.parameters.get<mio::abm::InfectedSymptomsToSevere>()      = 6.;
+    model1.parameters.get<mio::abm::SevereToRecovered>()             = 8.;
+    model1.parameters.get<mio::abm::SevereToCritical>()              = 7.;
+    model1.parameters.get<mio::abm::CriticalToRecovered>()           = 10.;
+    model1.parameters.get<mio::abm::CriticalToDead>()                = 11.;
+
+    //Age group 0 goes to school and age group 1 goes to work
+    model1.parameters.get<mio::abm::AgeGroupGotoSchool>()[age_group_children] = true;
+    model1.parameters.get<mio::abm::AgeGroupGotoWork>()[age_group_adults]     = true;


I think these are supposed to be for model2?

DavidKerkmann · 2025-01-06T13:49:54Z

cpp/models/abm/person_id.h

    {
    }

    /// @brief Create an invalid ID.
    PersonId()
-        : mio::TypeSafe<uint32_t, PersonId>(std::numeric_limits<uint32_t>::max())
+        : mio::TypeSafe<uint64_t, PersonId>(std::numeric_limits<uint32_t>::max())


We quickly talked about the change to 64 bit.
A comment somewhere here would be good to explain that only valid uint32_t values should be used for the Id. Perhaps even an assert in the constructor?

DavidKerkmann · 2025-01-06T13:58:43Z

cpp/models/graph_abm/graph_abm_mobility.h

+    }
+
+private:
+    Sim m_simulation; ///< ABM Simulation of the node


Just as a comment:
This is interesting. I would have thought that each node is the Model, not holding a simulation, and that there is only one simulation that determines how to advance globally.
Holding a simulation for each node however allows for (potential) different time steps, and it is in line with the ODE models. (And allows for refined histories, individual results, etc.)

DavidKerkmann · 2025-01-06T14:10:42Z

cpp/models/abm/model.cpp

-            mio::abm::add_exposure_contribution(m_air_exposure_rates_cache[location],
-                                                m_contact_exposure_rates_cache[location], person,
-                                                get_location(person.get_id()), t, dt);
+            if (person.get_location_model_id() == m_id) {


Can we also check with the activeness status here to be consistent?

DavidKerkmann · 2025-01-07T14:48:26Z

cpp/models/graph_abm/graph_abm_mobility.h

+ * @param[in] node ABMSimulationNode to which the functor is applied.
+ */
+template <class... History>
+void evolve_model(abm::TimePoint t, abm::TimeSpan dt, ABMSimulationNode<History...>& node)


See naming comment above. Perhaps: advance_node?

DavidKerkmann · 2025-01-07T14:52:00Z

cpp/tests/test_graph_abm.cpp

+struct MockHistory {
+
+    template <class T>
+    void log(const T& t)
+    {
+        mio::unused(t);
+    }
+};


Could be omitted if the changes for the history are accepted.
Perhaps one needs to test the alternative advance with a history, though.

DavidKerkmann · 2025-01-07T14:54:56Z

cpp/tests/test_graph_abm.cpp

+    graph.add_edge(0, 1);
+    graph.add_edge(1, 0);


Shouldn't the Ids be used here, too?

DavidKerkmann · 2025-01-07T15:06:34Z

cpp/tests/test_graph_abm.cpp

+    EXPECT_EQ(node2.get_simulation().get_model().get_persons().size(), 0);
+    EXPECT_EQ(node1.get_simulation().get_model().get_persons().size(), 4);
+    EXPECT_EQ(node1.get_simulation().get_model().get_activeness_statuses()[p2_index], false);
+    EXPECT_EQ(node1.get_simulation().get_model().get_activeness_statuses()[p4_index], false);


One could also check the buffer sizes at this point, perhaps not 100% needed, you can decide.

DavidKerkmann · 2025-01-07T15:10:00Z

cpp/models/graph_abm/model_wrapper.h

+namespace mio
+{
+using namespace abm;
+class ModelWrapper : public abm::Model


I'm a bit unhappy with the naming, as writing ModelWrapper in the graph initialization is not so nice in my opinion. I'd like this to also be called Model, but of course this is a problem, unless we use a different namespace here. Maybe we can discuss this. Options:

Keep the name

Rename namespace to abm_graph or similar

Rename to Model and rename abm::Model to something else

Co-authored-by: David Kerkmann <[email protected]>

reneSchm · 2025-01-10T13:51:51Z

cpp/models/abm/person.cpp

+Person::Person(const Person& other, PersonId id, uint64_t unique_id)
+    : Person(other)
+{
+    m_person_id = id;
+    m_unique_id = unique_id;
+}
+


So we have now two IDs, one of which does not always ID(entify) a person? This needs at least a change in naming or documentation.

As an idea, maybe use local/global in the name of the ID?
Maybe than it could make sense that the global ID defaults to INVALID_ID. And when it is invalid, get_global_id can fall back to the local ID, making them "the same" automatically?

I think a change in naming is a good idea, we can discuss that.

To the second part: If I understand you right, this is exactly what is done in "add_person".

Kind of... But I think we should decide either on the World always overwriting the unique ID (like it does with PersonID), or on the World not changing it at all. I feel like changing the ID only when it is invalid can lead to unexpected behaviour, as invalid now means "not set, please overwrite" instead.

We cannot always overwrite it, because when a person changes the world, then it would lose its unique id. What we need to do is only set it, if it has not been set before. I did this using the INVALID_UNIQUE_ID, but I'm open for other solutions :).

I think the global ID is meant to be set only once, when the person first enters the simulations and is assigned a "home world". For me, this changes make sense. Best would be to have the global ID const, which is initialized during the Person constructor, but this would change the add_person structure. We could make some struct that only allows for a one-time modification and then being const. But there might be better solutions.

What we need to do is only set it, if it has not been set before. I did this using the INVALID_UNIQUE_ID

This works now, but the reason we have this invalid id is to signal that something went wrong, hence all the asserts checking for it. If you use it additionally as a "not set" flag, this can potentially end up hiding other problems.

I would be in favor of making it effectively const - an actually const member would be annoying, as we cannot use default move or copy ctors.
Some options would be:

Setting the UID in main, and removing the UID overwrite from add_person and that Person ctor which copies a Person and assigns a new ID

using an optional (so we have a "not set" state)

defining a second value like invalid_id
The latter two options then require checking that the id is both set and valid every time we pass it to a new function. The first requires additional setup outside of the model (which is okay in theory, as it is a "global" id).

All are a bit weird in that we treat the IDs differently. Only one is always being overwritten by the world, the other only sometimes, or elsewhere.

cpp/models/abm/model.cpp

jubicker added 28 commits June 7, 2024 10:02

add ABM simulation node and edge

f272d9c

graph abm implementation

1f29e1c

remove warning surpression for boost

5e91137

merge main

22a2a60

add CMakeList

4368cb5

CMake List Bug fix

81df1a8

fix include

5b2c64a

bug fix

414c3f4

test

4e2a733

test apply_mobility

8f1d7cb

extend apply_mobility test

7400802

error in abm_minimal example

b314715

test for apply mobility

dbbd068

finx clang bug

196275f

abm minimal bug fix

a5a4e88

Merge branch '646-use-graph-model-with-abms-v2' of https://github.com…

3367fa6

…/SciCompMod/memilio into 646-use-graph-model-with-abms-v2

graph_abm example

8cce390

graph abm example

2fc9026

merge main

ef4c0b1

fix merge conflicts

0ece6be

fix test

b773193

graph abm example

b4d44d5

remove unnecessary comment

ae95301

Merge branch '646-use-graph-model-with-abms-v2' of https://github.com…

5ebd62f

…/SciCompMod/memilio into 646-use-graph-model-with-abms-v2

fix bug

e22009b

delete dbugging comments

11c3ae8

Merge branch '646-use-graph-model-with-abms-v2' of https://github.com…

1f85548

…/SciCompMod/memilio into 646-use-graph-model-with-abms-v2

benchmark inactive persons

fdad4da

jubicker linked an issue Aug 1, 2024 that may be closed by this pull request

Use graph model with abms #646

Open

2 tasks

merge main

509854c

jubicker added 3 commits December 3, 2024 15:03

merge main

58d4bb1

Debug logs

1e69b4d

Merge branch '646-use-graph-model-with-abms-v3' of https://github.com…

0a822ef

…/SciCompMod/memilio into 646-use-graph-model-with-abms-v3

DavidKerkmann requested changes Jan 7, 2025

View reviewed changes

jubicker and others added 4 commits January 8, 2025 16:36

Update cpp/examples/graph_abm.cpp

1eabac2

Co-authored-by: David Kerkmann <[email protected]>

Apply suggestions from code review

944736f

Co-authored-by: David Kerkmann <[email protected]>

merge main

ecdd68a

add unique person id

26fe99f

reneSchm reviewed Jan 10, 2025

View reviewed changes

jubicker added 4 commits January 10, 2025 15:28

review comment

66896aa

fix CI

8a97b5c

bug fix

a106505

remove const from id getter

34ebe3b

DavidKerkmann reviewed Jan 13, 2025

View reviewed changes

cpp/models/abm/model.cpp Outdated Show resolved Hide resolved

DavidKerkmann changed the title ~~646 use graph model with abms v3~~ 646 use graph model with ABMs v3 Jan 13, 2025

jubicker added 15 commits January 14, 2025 09:26

sort m_persons vector before moving persons between nodes

f21f37f

unique id

6a76553

extend tests

e0f3be1

fix test

788724a

[CI Skip] test

a807dd0

[ci skip]

7a4d6d6

test

17ff528

test

c3c92c5

ci

c7e60a8

ci

f221fb3

fix CI

bfb7b72

Rename model_wrapper to graph_abmodel

ad4a5fd

rename evolve to advance in graph

dd58d68

introduce local index and global id

3eaa51a

add tests

830aff3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

646 use graph model with ABMs v3 #1085

646 use graph model with ABMs v3 #1085

jubicker commented Aug 1, 2024 •

edited by DavidKerkmann

Loading

DavidKerkmann left a comment

DavidKerkmann Jan 6, 2025

DavidKerkmann Jan 6, 2025

DavidKerkmann Jan 6, 2025

DavidKerkmann Jan 6, 2025

DavidKerkmann Jan 7, 2025

DavidKerkmann Jan 7, 2025

DavidKerkmann Jan 7, 2025

DavidKerkmann Jan 7, 2025

DavidKerkmann Jan 7, 2025

reneSchm Jan 10, 2025

jubicker Jan 10, 2025 •

edited

Loading

reneSchm Jan 10, 2025

jubicker Jan 10, 2025 •

edited

Loading

DavidKerkmann Jan 13, 2025

reneSchm Jan 13, 2025

646 use graph model with ABMs v3 #1085

Are you sure you want to change the base?

646 use graph model with ABMs v3 #1085

Conversation

jubicker commented Aug 1, 2024 • edited by DavidKerkmann Loading

Changes and Information

Merge Request - Guideline Checklist

Checks by code author

Checks by code reviewer(s)

DavidKerkmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubicker Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubicker Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jubicker commented Aug 1, 2024 •

edited by DavidKerkmann

Loading

jubicker Jan 10, 2025 •

edited

Loading

jubicker Jan 10, 2025 •

edited

Loading